Skip to content

Conversation

@anzr299
Copy link
Collaborator

@anzr299 anzr299 commented Sep 22, 2025

Changes

Introduced a new API to offer weights compression algorithm for quantizers defined in torch.ao.
Currently only supports OpenVINO Quantizer.

Reason for changes

To support Quantizers defined in torch ao.

Related tickets

169342

@anzr299 anzr299 requested a review from a team as a code owner September 22, 2025 14:43
@github-actions github-actions bot added the API Public API-impacting changes label Sep 22, 2025
@anzr299 anzr299 marked this pull request as draft September 22, 2025 14:56
@daniil-lyakhov daniil-lyakhov self-requested a review September 22, 2025 15:03
Copy link
Collaborator

@daniil-lyakhov daniil-lyakhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I see the PR with OpenVINOQuantizer?

Comment on lines 34 to 35
) -> torch.fx.GraphModule:
self._quantizer = quantizer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typehints an docstring are missing

@daniil-lyakhov daniil-lyakhov self-requested a review November 5, 2025 12:15
Copy link
Collaborator

@nikita-savelyevv nikita-savelyevv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge work, thanks @anzr299!

Mostly minor comments from my side. Overall the updated approach in src/nncf/quantization/algorithms/weight_compression/algorithm.py looks good in my opinion and does not change the logic of the algorithm.

The only significant difference I noticed is that ratio_defining_params are initialized with primary_config from the start, and then some of the parameters are converted back to backup precision after mixed precision algorithm. Before, it was the other way around. It looks a bit cumbersome during mixed precision assignment, but allows to avoid passing group_size_values which is an improvement compared to the previous approach.

quantizer_builder: Callable[..., OpenVINOQuantizer],
model_case: ModelCase,
quantizer_params,
pt2e_params,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that in this and some other cases below, pt2e_params argument is not used. Is this on purpose? Won't this result in unnecessary duplication of tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially left it there since all tests had a common fixture with TEST_MODELS. I have modified it now though, to use a different list to get arguments for test cases where pt2e_params is not required.

Copy link
Collaborator

@nikita-savelyevv nikita-savelyevv Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please correct me if I'm wrong, but with TEST_MODELS_NO_PT2E defined as [(m, qparams) for m, qparams, _ in TEST_MODELS], it will still contain repeating entities

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes you're right 🤦
it wasnt visible with current cases since there is only 1 element in the pt2e list.
Done

Copy link
Contributor

@ljaljushkin ljaljushkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome feature!

model, graph, statistic_points, dataset, ratio_defining_params, all_weight_params
)
# Apply Mixed precision algorithm to ratio defining parameters
self._algo._apply_mixed_precision(ratio_defining_params, model, graph, statistic_points)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really minor and not blocking. I'd remove underscore in the name, since methods are used externally.

Copy link
Collaborator Author

@anzr299 anzr299 Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes great point

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API Public API-impacting changes NNCF PT Pull requests that updates NNCF PyTorch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants